Skip to content

Allow --json option for list#128

Open
muellerj wants to merge 1 commit intoborgbase:mainfrom
muellerj:list-json
Open

Allow --json option for list#128
muellerj wants to merge 1 commit intoborgbase:mainfrom
muellerj:list-json

Conversation

@muellerj
Copy link
Copy Markdown

@muellerj muellerj commented May 1, 2026

This is my first attempt at the JSON output mentioned in #35. Please advise if you'd rather pull these into AsciiTablePrinter and JsonPrinter respectively - I don't think it's neccessary for list.

Also please let me know if you've got a "larger plan" for the JSON output or if you're fine with adding branches for each command.

@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented May 2, 2026

Thanks! Will check it out.

@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented May 3, 2026

Thanks Jonas — happy to see this picked up. A couple of things to sort out before we merge, mostly about making the JSON useful for the scripting use case from #35.

Main concern: the JSON should be structured data, not pre-formatted strings.

Right now the values are the same human-formatted strings the table prints, e.g.:

{ "Date": "2026-01-15 14:30", "Source": "/srv/a\n/srv/b", "Files": "1.2k", "Size": "11 B", "Host": "" }

A consumer would have to reparse "11 B" back to bytes, "1.2k" is lossy for format_count, "Date" is local-time without seconds or offset, Source is a newline-joined string instead of an array, and Host/Label are blanked on rows that share the previous group (a visual table trick that doesn't translate to JSON — every record should be self-contained).

Both SnapshotEntry and SnapshotStats already derive(Serialize), so a more useful (and shorter) version is roughly:

serde_json::to_string_pretty(&snapshots)?  // Vec<(SnapshotEntry, Option<SnapshotStats>)>

That gives you ISO-8601 timestamps from chrono, the snapshot id, the full source_paths array, and all stats fields including the ones the table currently drops. If you'd rather pin a stable wire schema, a small CLI-side #[derive(Serialize)] view struct works and is still cleaner than json!. Either way: snake_case keys, raw ints for sizes/counts, null for missing, arrays for paths.

The test would be a bit stronger if it round-trips through serde_json::from_str::<Value> and asserts a couple of fields, instead of substring-matching "Size": "11 B".

Two questions on the larger plan, both worth deciding before this lands so we don't end up with seven copies of the same branching:

  1. Per-command --json vs global --output {table,json} (or top-level --json)? I'd lean global — it covers the bare-command path too and avoids re-declaring on every subcommand. Curious what you prefer.
  2. Extract a tiny Printer abstraction now, or defer until the second command? From Feature request: --json for info, list #35 the obvious next ones are info, snapshot info, snapshot list, snapshot diff, snapshot find. If you want to land just list first under whichever flag style we pick and then we extract a printer when adding info, that's also fine — just want to agree on direction.

No new third-party dep concerns — serde_json is already a workspace dep used by vykar-gui, vykar-server, and vykar-protocol.

@muellerj
Copy link
Copy Markdown
Author

muellerj commented May 4, 2026

Thanks for the guidance - these all make sense. I'll see if I can work on these towards the end of this week.

Concering direction - if all the subcommands want to have a --json sooner or later, global seems inevitable to me. I feel like a top-level --json (default false) is the path of least suprise. --output {table,json} feels a bit clunky.

Depending on the representation of the values, the Printer abstraction might not be as lean as I thought it could be. As you rightfully noted, the keys and values may differ significantly between the output for a human and the JSON. I'd start with spaghetti-code-style for list and see how easy it is to extract two printers from there if that sounds good to you?

@muellerj muellerj force-pushed the list-json branch 2 times, most recently from e18d01c to c6fc586 Compare May 4, 2026 12:27
use crate::passphrase::with_repo_passphrase;
use crate::table::CliTableTheme;

// use serde_json::Value;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels out of place here. Where would you place SnapshotView?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants